-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Still more updates to support Volume layer activities. #1508
base: main
Are you sure you want to change the base?
Conversation
Two new structs to describe a Volume: pub struct VolumeInfo { pub block_size: u64, pub volumes: Vec<SubVolumeInfo>, } pub struct SubVolumeInfo { pub blocks_per_extent: u64, pub extent_count: u32, } Added a new method to the Volume type that will gather extent info from each sub-volume and return a VolumeInfo struct that describes the whole volume. In the BlockIO trait, replaced query_extent_size with query_extent_info. Objects that support this call will return the RegionExtentInfo type filled out. Things that do not will just return None. Updated crutest's RegionInfo struct to have a VolumeInfo field. Updated fill_sparse and span crutest tests to perform their work on all sub-volumes. print_region_description looks like this: ---------------------------------------------- random read with 4 KiB chunks (1 block) region info: block size: 4096 bytes sub_volume 0 blocks / extent: 100 sub_volume 0 extent count: 15 sub_volume 0 extent size: 400 KiB sub_volume 1 blocks / extent: 100 sub_volume 1 extent count: 15 sub_volume 1 extent size: 400 KiB total blocks: 3000 total size: 11.7 MiB encryption: no ---------------------------------------------- Added fill-sparse test to crutest cli Coming in a follow up PR I would like to rename RegionInfo in crutest to be DiskInfo. This would just be name changes, no functional changes. I decided to break that out to reduce review overhead.
@@ -1071,9 +1100,12 @@ pub async fn start_cli_server( | |||
* If write_log len is zero, then the RegionInfo has | |||
* not been filled. | |||
*/ | |||
let volume_info = VolumeInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be pulling this info from the volume
argument? It's not obvious why initializing it with fictional values is okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was that way because I could not figure out how to make in an Option
and pass it back and forth between process_cli_command
and start_cli_server
. Let me see if I can figure it out now
let mut ri: RegionInfo = RegionInfo { | ||
block_size: 0, | ||
extent_size: Block::new_512(0), | ||
volume_info, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove total_size
and total_blocks
here, because they can both be calculated from volume_info
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon further reading, it would minimize the diff to also add a block_size()
helper function, since lots of the diff in this PR is s/block_size/volume_info.block_size
.
fn block_size(&self) -> u64 {
self.volume_info.block_size
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked the idea of computing it once, then just passing it around, but don't really care if we compute it every time we need it. I don't think its anywhere inside the performance path, and if I find a place I'll handle it there.
"[{index}][{extent}/{extents}] Write to block {}", | ||
block_index | ||
); | ||
volume.write(offset, data).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right to me, because offset
here is a relative offset within the subvolume (but ignores the offset of that subvolume within the whole volume).
I think we need to accumulate a global offset (cumulative sum of sv.extent_count * sv.blocks_per_extent
) and add it to offset
here.
let mut es = 0; | ||
for sv in ri.volume_info.volumes.iter() { | ||
es += sv.blocks_per_extent; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense to me; the sum of blocks-per-extent across all subvolumes isn't a meaningful value.
In a subvolume-flavored world, there is no single extent size (es
), so we should just remove it. Extent count ec
remains meaningful, but should be a function on VolumeInfo
, e.g.
impl VolumeInfo {
/// Returns the total number of extent files in this volume
fn extent_count(&self) -> usize {
self.subvolumes.iter().map(|s| s.extent_count).sum().unwrap_or(0)
}
}
Flush, | ||
Generic(usize, bool), | ||
Info(u64, u64, u64), | ||
Info(VolumeInfo, u64, usize), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just pass VolumeInfo
, because total size and total blocks can be calculated from it.
pub async fn query_extent_size(&self) -> Result<Block, CrucibleError> { | ||
self.send_and_wait(|done| BlockOp::QueryExtentSize { done }) | ||
.await | ||
pub async fn query_extent_info( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this implemented twice (once in impl Guest
and once in impl BlockIO for Guest
)?
blocks_per_extent: rd.extent_size().value, | ||
extent_count: rd.extent_count(), | ||
}; | ||
done.send_ok(ei); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit the warning messages below to say "extent info not available" (instead of "extent size")?
for sub_volume in &self.sub_volumes { | ||
match sub_volume.query_extent_info().await? { | ||
Some(ei) => { | ||
assert_eq!(self.block_size, ei.block_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment here as to why this can never happen (is it checked on Volume construction?)
volumes.push(svi); | ||
} | ||
None => { | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would lean towards "mixing extent and non-extent flavored subvolumes is an error", instead of ignoring it.
(In other words, return an error if we get a None
for a subvolume and !volumes.is_empty()
)
This is a re-do, re-work of what was in: Make crutest know extent info for each sub volume. #1474
Two new structs to describe a Volume:
Added a new method to the
Volume
type that will gather extent info from each sub-volume and return aVolumeInfo
struct that describes the whole volume.In the
BlockIO
trait, replacedquery_extent_size
withquery_extent_info
. Objects that support this call will return theRegionExtentInfo
type filled out. Things that do not will just returnNone
.Updated crutest's
RegionInfo
struct to have aVolumeInfo
field.Updated the fill_sparse and span crutest tests to perform their work on all sub-volumes.
print_region_description looks like this:
Added fill-sparse test to crutest cli
Coming in a follow up PR I would like to rename
RegionInfo
in crutest to beDiskInfo
. This would just be name changes, no functional changes. I decided to break that out to reduce review overhead.